wasapi: detect device changes and handle disconnection gracefully#2538
wasapi: detect device changes and handle disconnection gracefully#25380x0003 wants to merge 2 commits into
Conversation
Catch exceptions from GetVolume()/SetVolume() so that a disconnected device (AUDCLNT_E_DEVICE_INVALIDATED) degrades gracefully instead of crashing MPD. Closes MusicPlayerDaemon#1880
Register an IMMNotificationClient to receive OnDefaultDeviceChanged callbacks. On change, the output thread wakes and reopens on the new default device immediately, bypassing the 10-second fail timer via a new AudioOutput::IsDeviceChange() virtual. Closes MusicPlayerDaemon#1408 Closes MusicPlayerDaemon#1880
| "Unable to get master " | ||
| "volume level"); | ||
| try { | ||
| auto future = com_worker->Async([&]() -> int { |
There was a problem hiding this comment.
You moved this into the try block, but what exception will this catch?
| LogDebug(wasapi_mixer_domain, | ||
| "Failed to get volume, device may be disconnected"); |
There was a problem hiding this comment.
This is just a debug message and you're discarding the actual error details.
| return; | ||
|
|
||
| try { | ||
| com_worker->Async([&]() { |
There was a problem hiding this comment.
See above. The diff is awfully large and that needs to be justified.
MaxKellermann
left a comment
MaxKellermann
left a comment
There was a problem hiding this comment.
I didn't understand much of this because I'm not a Windows guy, but some things looked exceptionally ugly.
| const UINT32 frame_size; | ||
| const UINT32 buffer_size_in_frames; | ||
| const bool is_exclusive; | ||
| const bool is_exclusive; |
| * device changes on Windows. | ||
| */ | ||
| class WasapiDeviceNotification final : public IMMNotificationClient { | ||
| std::atomic_bool &device_changed; |
There was a problem hiding this comment.
A reference to an atomic? That is truly ugly. Can you do better?
| /* Default output device changed; the output | ||
| framework will reopen on the new device. */ |
There was a problem hiding this comment.
But what if we're not using the default device?
| framework will reopen on the new device. */ | ||
| LogDebug(wasapi_output_domain, | ||
| "Audio device changed, stopping playback"); | ||
| throw std::runtime_error("Audio device changed"); |
There was a problem hiding this comment.
To trigger InternalCloseError and IsDeviceChange(), that's why the latter was introduced in the first place. It's not a "real" error. Without it there's a few seconds worth of delay (REOPEN_AFTER in Control.cxx) before audio stream resumes after another device disconnects.
Changing it would require adding a new reopen mechanism to the output framework. Maybe I'm just dumb and there's better/simpler way, but this is the best I could come up with.
| HANDLE handles[2] = { event.handle(), device_event }; | ||
| WaitForMultipleObjects(2, handles, FALSE, INFINITE); | ||
|
|
||
| if (device_changed.load(std::memory_order_acquire)) { |
There was a problem hiding this comment.
If you'd use the return value of WaitForMultipleObjects(), this could be implemented without the atomic_bool, couldn't it?
| * output framework may use this to skip the fail timer and | ||
| * reopen the output immediately. | ||
| */ | ||
| virtual bool IsDeviceChange() const noexcept { |
There was a problem hiding this comment.
This is not an elegant design. If you really wanted to signal this kind of thing, you'd better throw a certain exception class, this would still be ugly but somewhat less ugly. But I have a feeling a better design would be possible.
There was a problem hiding this comment.
See comment on WasapiOutputPlugin.cxx:460-463. I'll see if I can figure out another way, but I'm kind of at my wit's end here.
2 participants
Closes #1408
Closes #1880
Changes were made to Control.hxx, Interface.hxx, Thread.cxx, which are technically a bit out of scope as those are generic, but I couldn't figure out how to make it work some other way.
Their behavior essentially stays the same though, so no other platforms should be affected.